-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run the focus pass in post-processing for all events #604
Conversation
I didn't catch this before because the NVDA command for moving the input focus to the screen reader cursor happens to include the shift key, which triggered a text event. But Narrator's behavior was broken before, along with the WIP AccessKit Android adapter. And, even more basic than that, textboxes weren't actually receiving focus immediately when clicked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just be part of post_event_processing
instead, based on my reading of linebender/rfcs#7
OK, moved it into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this needs to be slightly further down in post_event_processing. cc @PoignardAzur
masonry/src/render_root.rs
Outdated
@@ -484,6 +483,8 @@ impl RenderRoot { | |||
|
|||
// --- MARK: POST-EVENT --- | |||
fn post_event_processing(&mut self, widget_state: &mut WidgetState) { | |||
run_update_focus_pass(self, widget_state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditions under which run_update_focus_pass
needs to happen are a bit unclear, and I suspect that logic should live inside the pass itself.
I think it only does something if:
- Reparenting happens (impossible)
- The currently focused node is now disabled
- The next focused node is different to the current focused node.
I think this should move to after the update_focus_chain
pass as a first pass, with a comment like:
// TODO: Only run this pass if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it at the top because root_on_text_event
used to make that call before post_event_processing
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed. This is why I've asked Olivier to weigh in.
I would approve optimistically if this were moved to be after update_focus_chain
, presuming that testing still works.
I would recommend running that pass twice: once in the event function and once after update focus chain like Daniel says. Ultimately we should add a fixpoint loop that repeats all passes as per the RFC. |
@PoignardAzur it might make sense for you to just push that change |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solves the issue mentioned on Zulip.
Of course, future work here is expected
Requesting and resigning focus can happen when handling pointer and accessibility events, not just keyboard events.